Skip to content

[SR-2694] Cannot JSON serialize array with boolean value #648

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 28, 2016

Conversation

saiHemak
Copy link
Contributor

@saiHemak saiHemak commented Sep 19, 2016

@saiHemak saiHemak changed the title Cannot JSON serialize array with boolean value - bug [SR-2694] Cannot JSON serialize array with boolean value Sep 19, 2016
@modocache
Copy link
Contributor

LGTM, but I defer to people who know corelibs-foundation well, like @phausler or @parkera.

@swift-ci please test

@@ -285,6 +285,8 @@ private struct JSONWriter {
try serializeDictionary(dict)
} else if let null = obj as? NSNull {
try serializeNull(null)
} else if let boolVal = obj as? Bool {
try serializeNumber(NSNumber(booleanLiteral: boolVal))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NSNumber(value: boolVal) would be better since you are not passing a literal here

@saiHemak saiHemak force-pushed the jsonserialize-branch branch from 1e6d34c to 47790e1 Compare September 20, 2016 03:52
@saiHemak
Copy link
Contributor Author

@phausler ..Thanks for the review..I have modified the code as per your comments .Please review

func test_booleanJSONObject() {
do {
let mydata = try JSONSerialization.data(withJSONObject: [true])
XCTAssertEqual(String(data: mydata, encoding: String.Encoding.utf8),"[1]")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing whitespace between arguments

@saiHemak saiHemak force-pushed the jsonserialize-branch branch from 47790e1 to 68d703e Compare September 27, 2016 08:48
@pushkarnk
Copy link
Member

@swift-ci please test

2 similar comments
@pushkarnk
Copy link
Member

@swift-ci please test

@parkera
Copy link
Contributor

parkera commented Sep 27, 2016

@swift-ci please test

@parkera
Copy link
Contributor

parkera commented Sep 28, 2016

@phausler if you're ok with this after the changes, please merge

@phausler phausler merged commit f4cb7cb into swiftlang:master Sep 28, 2016
@saiHemak saiHemak deleted the jsonserialize-branch branch October 3, 2016 04:18
pushkarnk pushed a commit to pushkarnk/swift-corelibs-foundation that referenced this pull request Oct 7, 2016
parkera pushed a commit that referenced this pull request Oct 7, 2016
* [SR-2694] Cannot serialize JSON object with array (#648)

* Support JSONSerialization of more number types (#650)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants